[ENG-603] Fix caching in care backend build actions#3696
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a plugin-hash resolver, switches reusable-test and deploy builds to GHCR-backed Buildx caches, passes the resolved hash into Docker builds, and adds a deploy job that deletes older GHCR build cache versions. ChangesBuild cache and plugin hash plumbing
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…n staleness mitigation
- Replace actions/cache + local buildx cache with type=registry GHCR cache
- Use weekly-rotated cache tags (buildcache-{platform}-{year-Wweek})
- Add prune-cache job to delete tags older than 4 weeks on develop
- Add resolve_plugins.py to compute PLUGIN_RESOLVED_HASH from @Branch refs
for cache busting when plugin upstreams change
- Add PLUGIN_RESOLVED_HASH ARG/ENV to both Dockerfiles
- Remove buildkit-cache-dance and all GHA cache boilerplate from test workflow
- Conditional cache-to in reusable-test.yml (write only on push)
tellmeY18
force-pushed
the
fix/workflow-cache
branch
from
9c55d2e to
9d9a3ea
Compare
Greptile SummaryThis PR replaces filesystem-based
Confidence Score: 4/5Safe to merge for the prod build path; the dev build cache write is broken but does not affect CI correctness, only cache effectiveness. The reusable-test.yml CACHE_TO write guard checks github.event_name which is always workflow_call in a reusable workflow, meaning the dev build cache is never populated in the registry. Every push to develop reads from but never refreshes the cache, leaving it permanently stale after the first cold miss. reusable-test.yml — the CACHE_TO write guard on line 47 needs to reference inputs.event_name instead of (or in addition to) github.event_name. Important Files Changed
Reviews (4): Last reviewed commit: "Address review comments on plugin cache ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/reusable-test.yml (1)
24-29: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: unpinned
docker/login-action@v3.zizmor flags Line 25 as unpinned per a blanket-hash policy. It's consistent with the rest of this workflow's tag-pinned actions, so this is only worth addressing if you intend to enforce SHA pinning repo-wide.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-test.yml around lines 24 - 29, The GitHub Container Registry login step uses an action pinned only by tag, which may violate the repo’s SHA-pin policy. If this workflow should follow the same enforcement as the other tag-pinned actions, update the Docker login step in the reusable test workflow to use a fixed commit SHA for docker/login-action instead of `@v3`, keeping the existing Login to GitHub Container Registry block and its inputs unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/resolve_plugins.py:
- Around line 26-42: The plugin ref resolution in resolve_plugins.py is silently
falling back to the raw branch/ref when git ls-remote fails or returns no
output, which makes values like pkg@main stop changing and hides resolution
problems; update the logic around the ref handling to surface the failure
instead of assigning sha from ref. Also fix the version defaulting in the
package version lookup so a null version is treated like missing input by using
the same default path as the versionless case, and make sure the
ver.startswith("@") branch in the resolver no longer crashes on null values.
In @.github/workflows/reusable-test.yml:
- Around line 42-54: The build step in the reusable workflow is passing
ADDITIONAL_PLUGS directly from the GitHub expression into the shell, which can
strip JSON quotes and split the value before docker buildx receives it. Move
ADDITIONAL_PLUGS into the step env and reference it as a quoted shell variable
in the Build images run block, using the existing docker buildx build invocation
and keeping PLUGIN_RESOLVED_HASH unchanged.
In `@docker/prod.Dockerfile`:
- Around line 37-40: The builder-stage plugin install step in
docker/prod.Dockerfile is not using PLUGIN_RESOLVED_HASH, so changes to the
resolved plugin set do not invalidate the install_plugins.py layer. Update the
RUN step that invokes install_plugins.py to reference PLUGIN_RESOLVED_HASH
alongside the existing ARGs, using the same pattern as the corresponding
dev.Dockerfile cache-busting fix, so the layer is rebuilt whenever the hash
changes.
---
Nitpick comments:
In @.github/workflows/reusable-test.yml:
- Around line 24-29: The GitHub Container Registry login step uses an action
pinned only by tag, which may violate the repo’s SHA-pin policy. If this
workflow should follow the same enforcement as the other tag-pinned actions,
update the Docker login step in the reusable test workflow to use a fixed commit
SHA for docker/login-action instead of `@v3`, keeping the existing Login to GitHub
Container Registry block and its inputs unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8740dfbc-e1d4-4a9b-896b-a767c37f5131
📒 Files selected for processing (5)
.github/scripts/resolve_plugins.py.github/workflows/deploy.yml.github/workflows/reusable-test.ymldocker/dev.Dockerfiledocker/prod.Dockerfile
| if ver.startswith("@"): | ||
| ref = ver[1:] | ||
| git_url = pkg.removeprefix("git+") | ||
| try: | ||
| out = subprocess.run( # noqa: S603 | ||
| ["git", "ls-remote", git_url, ref], # noqa: S607 | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=15, | ||
| ) | ||
| sha = out.stdout.split()[0] if out.stdout else ref | ||
| except Exception: | ||
| sha = ref | ||
| resolved.append(f"{pkg}@{sha}") | ||
| else: | ||
| resolved.append(f"{pkg}{ver}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/scripts/resolve_plugins.py')
print(p.exists(), p)
print(p.read_text())
PYRepository: ohcnetwork/care
Length of output: 1395
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/scripts/resolve_plugins.py')
print(p.exists(), p)
print(p.read_text())
PYRepository: ohcnetwork/care
Length of output: 1395
Avoid falling back to the raw ref here. When git ls-remote fails or returns nothing, sha becomes ref, so @main hashes like pkg@main and stops busting the cache when the branch moves. Surface the failure instead of quietly pretending it resolved.
Also, p.get("version", "@main") does not cover "version": null; that still reaches ver.startswith("@") and crashes. Use p.get("version") or "@main" if null should mean default.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 29-35: Command coming from incoming request
Context: subprocess.run( # noqa: S603
["git", "ls-remote", git_url, ref], # noqa: S607
check=False,
capture_output=True,
text=True,
timeout=15,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.18)
[warning] 38-38: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/resolve_plugins.py around lines 26 - 42, The plugin ref
resolution in resolve_plugins.py is silently falling back to the raw branch/ref
when git ls-remote fails or returns no output, which makes values like pkg@main
stop changing and hides resolution problems; update the logic around the ref
handling to surface the failure instead of assigning sha from ref. Also fix the
version defaulting in the package version lookup so a null version is treated
like missing input by using the same default path as the versionless case, and
make sure the ver.startswith("@") branch in the resolver no longer crashes on
null values.
Source: Linters/SAST tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3696 +/- ##
===========================================
+ Coverage 79.55% 79.66% +0.10%
===========================================
Files 479 479
Lines 22996 23010 +14
Branches 2378 2379 +1
===========================================
+ Hits 18295 18330 +35
+ Misses 4096 4080 -16
+ Partials 605 600 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/scripts/resolve_plugins.py (1)
38-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlease fail closed here instead of reusing the ref name.
This still turns a
git ls-remotefailure into a stable cache key, so branch-based plugins likepkg@mainstop invalidating the image cache when the branch moves. Catching broadExceptionmakes that even easier to miss, which is… convenient in the wrong way. Raise on resolution failure here rather than warning and continuing with the raw ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/resolve_plugins.py around lines 38 - 39, The fallback in resolve_plugins.py should not warn and continue with the raw ref name when git ls-remote fails, because that creates a stable cache key for moving branches. Update the exception handling around the git resolution logic to fail closed by raising on resolution failure instead of returning or reusing the ref name; keep the change localized near the git_url lookup and the existing except Exception as exc block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/scripts/resolve_plugins.py:
- Around line 38-39: The fallback in resolve_plugins.py should not warn and
continue with the raw ref name when git ls-remote fails, because that creates a
stable cache key for moving branches. Update the exception handling around the
git resolution logic to fail closed by raising on resolution failure instead of
returning or reusing the ref name; keep the change localized near the git_url
lookup and the existing except Exception as exc block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 513a5f1f-6cf6-43d8-ba6d-5e1a47b3d3d0
📒 Files selected for processing (1)
.github/scripts/resolve_plugins.py
|
@tellmeY18 resolve comments. Lint failing |
- Quote ADDITIONAL_PLUGS build-arg to avoid shell word-splitting - Treat null plugin version as the default @main - Reference PLUGIN_RESOLVED_HASH in the install step so BuildKit invalidates the plugin layer when resolved plugin SHAs change
2 participants
Proposed Changes
actions/cache) with native BuildKit registry cache backed by GHCR (type=registry) in bothdeploy.ymlandreusable-test.ymlreproducible-containers/buildkit-cache-dancethird-party action dependencyresolve_plugins.pyscript to resolve plugin branch refs to commit SHAs viagit ls-remoteand produce a deterministic cache-busting hash as a Docker build arg (PLUGIN_RESOLVED_HASH)prune-cachejob to deletebuildcache-*GHCR package versions older than 4 weeks after each successfuldeveloppush--cache-toon fork PRs (skipped onpull_requestevents to avoid GHCR auth failures)Associated Issue
ENG-603
Summary by CodeRabbit